-
Notifications
You must be signed in to change notification settings - Fork 792
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[fpga] Add flow and constraints to dump the flash info array cell sites #25363
Conversation
CC @moidx Still a WIP, and I haven't tested splicing any particular image. We might be able to create a more targeted Tcl proc that could aggregate arrays (e.g. concatenate the two banks for each type into a linearized address space). Also, it looks like the bitstream cache handling will need some updating. There are two approaches I can think of:
|
Assuming the non-initialized pages remain in their current state (which I would assume is all 1's), is it possible to just initialize a portion of memory? Apparently we only need to deal with a handful of pages. |
I'm not sure what you mean here. The BRAM gets completely initialized when the bitstream is programmed, so we are always choosing the values. Right now, that is all ones, yes, representing erased flash cells. There is no partial loading at configuration time (i.e. when the bitstream is programmed into the FPGA). It's best to have the spliced image cover the entire memory, and program anything "unspecified" as all ones. Also, it's important to keep in mind that integrity is checked at the 64-bit datum granularity. |
6f66b14
to
f0f8695
Compare
Now I have collected all the memory maps into a single MMI file and adjusted all the cache handling and splicing scripts. |
27412a3
to
9f7c29e
Compare
Reduce BUILD file boilerplate by placing all memory map info in a single file. Use a separate Processor element for each map, so the InstData attribute can be used as a key to identify the memory to be updated. This construction also permits multiple memories to be spliced in a single call to updatemem, though that is not implemented in this commit. Adjust MMI dump to use a regex for mem type and a dictionary for each memory's parameters. For the mem type, some memories in the FPGA can consist of both RAMB36 and RAMB18 types, depending on the width of the array. The regex allows more flexible memory cell detection. Rev up the bitstream manifest schema to v3 so the memories contained in the MMI file are described. Add a function to collect MMI data from v2 cache entries and rewrite in the expected v3 format, with the MMI data in a single file. Finally, drop support for bitstream cache entries that do not contain a manifest. The tools to create a manifest have been in the repo for awhile, and as of this commit's creation, no such cache entries remain in the public cloud storage bucket. Signed-off-by: Alexander Williams <[email protected]>
For earlgrey, export the flash info memory maps to the MMI file. Add knowledge of these maps existing to the bitstream cache entries. The flash info arrays are identified by a key in this format: flash<FlashBank>_info<InfoType> Add KEEP_HIERARCHY to the prim_ram_1p instances representing the flash info arrays. This helps keep intelligible hierarchical paths to the memories, so we can readily select the correct cells. This lays groundwork for a future PR where we can splice these memories. Signed-off-by: Alexander Williams <[email protected]>
9f7c29e
to
e375d2b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps I ought to use these example files as test vectors, instead of making something else up inside the self-test Python script. But I'd punt that change for now ;)
"memories": [ | ||
"otp", | ||
"rom" | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This information isn't currently used in the bazel rules (not part of bitstreams_workspace.py), but I figured I'd make this info readily available in case it's useful.
The same info could be obtained from the MMI file directly, so if reviewers prefer I remove it from the manifest, I certainly can.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I think it is useful to know. In the future it would be interesting for us to consider creating a Bazel target that splices all memories in a single Bazel invocation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks @a-will, these changes are very useful! Some nice refactorings as well.
dict set memInfo otp fake_word_width 0 | ||
dict set memInfo otp addr_end_multiplier 16 | ||
|
||
# The flash banks have 76-bit wide words. 64 bits are data, and 12 bits are metadata. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will updatemem
support 76-bit wide words per line? It seems like it should, but I am curious to know if you have spotted any quirks when looking at the synthesis results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should, but I don't currently have an image that lets me check. Perhaps next we should cobble together something to test! :)
It at least doesn't complain about the presence of those memories in the MMI file.
"memories": [ | ||
"otp", | ||
"rom" | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I think it is useful to know. In the future it would be interesting for us to consider creating a Bazel target that splices all memories in a single Bazel invocation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alex, this seems to improve the MMI flow in addition to adding support for flash image uploading. I would only request you to make sure the docs about this area are adequate to navigate it for folks that need to work with images. This may be tricky since you are very familiar with it all, but things may look different if you pretend you don't :)
LGTM
Superseded by #25533 |
Adjust MMI dump to use a regex for mem type. Some memories in the FPGA can consist of both RAMB36 and RAMB18 types, depending on the width of the array. Use a regex to allow more flexible memory type detection.
Add KEEP_HIERARCHY to the prim_ram_1p instances representing the flash info arrays. This helps keep intelligible hierarchical paths to the memories, so we can readily select the correct cells. Because Vivado was previously combining all info types of the same bank into the same BRAMs, this increases BRAM utilization a bit.
Add calls to dump an MMI file for each array represented by aCollect all MMI data into a single file, and update the manifest and bazel rules to use it. Instead of having separate files, each map has a separate Processor element, and the InstData is assigned the key that identifies the memory (instead of using "dummy").(bank,type)
tuple, and add them to the bitstream cache entry.